Refactor helidon config 4.0 #4776
Conversation
|
/oca-checked |
|
/trigger |
|
Can we separate pico into its own dedicated PR ? |
romain-grecourt
changed the title
|
/trigger |
pico/api/pom.xml
Outdated
| <dependency> | ||
| <groupId>jakarta.inject</groupId> | ||
| <artifactId>jakarta.inject-api</artifactId> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
I don't understand idea of this.
Starting with 4.x jakarta.* annotations will be required for SE ?
There was a problem hiding this comment.
The jakarta.inject module is needed (as compile scope) for the definition of the Provider class, sadly in the same package with other annotations that would otherwise only be needed as provided scope.
trentjeff
force-pushed
the
pico-config
branch
from
35b9e74 to
5d9ad4d
Compare
common/common/pom.xml
Outdated
| @@ -1,7 +1,7 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <!-- | |||
|
|
|||
| Copyright (c) 2017, 2022 Oracle and/or its affiliates. | |||
| Copyright (c) 2022 Oracle and/or its affiliates. | |||
There was a problem hiding this comment.
This is wrong copyright change. It must be 2017, 2022. Please revert this change
| import java.util.Map; | ||
|
|
||
| /** | ||
| * <h2>Configuration</h2> |
There was a problem hiding this comment.
Adding headers to javadocs is tricky and not really needed here, when we have a single sentence description
| * @return current config node key | ||
| * @see #name() | ||
| */ | ||
| BaseKey key(); |
There was a problem hiding this comment.
BaseKey? This should return Key. Our API should be Key, BaseKey I would expect to be an abstract class I can extend, not API
| * @return {@code true} if the node is existing leaf node, {@code false} | ||
| * otherwise. | ||
| */ | ||
| boolean isLeaf(); |
There was a problem hiding this comment.
Not really needed. This is basically !isObject() && !isKey() && hasValue().
Also method hasValue() is missing
There was a problem hiding this comment.
There is no isKey() on either common/config/Config or config/config. I will add hasValue() decl on common/config.
There was a problem hiding this comment.
sorry, meant isList() not isKey()
| * @return a typed list with values | ||
| * @throws io.helidon.common.config.ConfigException in case of problem to map property value. | ||
| */ | ||
| default <T> ConfigValue<List<T>> asList(Class<T> type) throws ConfigException { |
There was a problem hiding this comment.
This should not have a default implementation. Esp. not an implementation like this
| * | ||
| * @see Config#key() | ||
| */ | ||
| interface BaseKey extends Comparable<BaseKey> { |
There was a problem hiding this comment.
This should be named Key. BaseKey would mean an abstract class. Also this is one of the main APIs of Config, so not prefixes should be used.
| * @throws IllegalStateException in case you attempt to call this method on a root node | ||
| * @throws io.helidon.common.config.ConfigException if not defined | ||
| */ | ||
| default BaseKey parent() throws ConfigException { |
There was a problem hiding this comment.
Do not add default implementations like this
| @@ -0,0 +1,21 @@ | |||
| /* | |||
| * Copyright (c) 2017, 2022 Oracle and/or its affiliates. | |||
There was a problem hiding this comment.
Copyright should be just 2022, this file did not exist in 2017
common/pom.xml
Outdated
| <module>configurable</module> | ||
| <module>reactive</module> |
There was a problem hiding this comment.
why is this reordered? Please simplify the change
| @@ -83,5 +83,20 @@ | |||
| <Method name="parse"/> | |||
| <Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/> | |||
| </Match> | |||
| <Match> | |||
| <!-- Path shadows common/config --> | |||
There was a problem hiding this comment.
what does Path mean in these descriptions? I do not understand this explanation
Fix for #4764